Skip to content

Feat: Create / Add Axios Large Response Interceptor #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 20 commits into from
Mar 6, 2025

Conversation

alexmarqs
Copy link
Member

An Axios interceptor designed to handle large responses.

  • Global and per-request options;
  • Custom callback function to fetch large payloads from a reference data, customisable reference property names, headers, and other options;

- Add comprehensive types for interceptor configuration
- Implement utility functions for large payload fetching
- Update interceptor to support dynamic configuration
- Add debug logging and error handling
- Expose new export for types and interceptor function
- Rename types and functions for clarity
- Add `enabled` option to conditionally apply interceptor
- Support custom large payload fetching strategy
- Update namespace and default options
- Enhance type safety and configuration flexibility
- Create comprehensive test cases for axios large response interceptor
- Add test configuration for Vitest
- Implement initial test scenarios for request and response interceptors
- Verify basic functionality and configuration options
- Add comprehensive test cases for global and custom configuration scenarios
- Verify interceptor behavior with different response types
- Improve test coverage for request and response interceptors
- Enhance test suite with more detailed assertions
- Add test cases for per-request and global option configurations
- Verify interceptor behavior with custom header flags and ref properties
- Improve test suite with more nuanced option merging and handling scenarios
- Expand assertions for large response interceptor functionality
@alexmarqs alexmarqs requested a review from jpinho February 28, 2025 12:30
@jpinho
Copy link
Member

jpinho commented Mar 4, 2025

Hey @alexmarqs, I have a fundamental request here, can we switch to not send large response accepts by default?

By default, it assumes that your backend uses @epilot/large-response-middleware

I think it should be the other way around. By default it should fail, it's not ok to have APIs return over 6MB of data, demonstrates likely a bad design in the code.

Would be nice for teams to inspect the failure and then decide to make it enabled:true. We should not have to do this (should be the default):

  axiosLargeResponse(client, {
    enabled: false,
    logger: Log,
  });

@alexmarqs
Copy link
Member Author

alexmarqs commented Mar 4, 2025

Hey @alexmarqs, I have a fundamental request here, can we switch to not send large response accepts by default?

By default, it assumes that your backend uses @epilot/large-response-middleware

I think it should be the other way around. By default it should fail, it's not ok to have APIs return over 6MB of data, demonstrates likely a bad design in the code.

Would be nice for teams to inspect the failure and then decide to make it enabled:true. We should not have to do this (should be the default):

  axiosLargeResponse(client, {
    enabled: false,
    logger: Log,
  });

Hey @jpinho I totally get your point, I thought about that when designing this but ended up with the default being enabled. Happy to have your feedback here and I will change this based on that 🙂!

@jpinho
Copy link
Member

jpinho commented Mar 4, 2025

Hey @alexmarqs, I have a fundamental request here, can we switch to not send large response accepts by default?

By default, it assumes that your backend uses @epilot/large-response-middleware

I think it should be the other way around. By default it should fail, it's not ok to have APIs return over 6MB of data, demonstrates likely a bad design in the code.
Would be nice for teams to inspect the failure and then decide to make it enabled:true. We should not have to do this (should be the default):

  axiosLargeResponse(client, {
    enabled: false,
    logger: Log,
  });

Hey @jpinho I totally get your point, I thought about that when designing this but ended up with the default being enabled. Happy to have your feedback here and I will change this based on that 🙂!

Yep, that also crossed my mind. It's just that enabling this will bloat our browsers caches, and slow down our MFEs greatly => in a super easy way.

We should log a warning for large responses on the client and make it default:false so that the change is explicit. Otherwise a distracted developer may start using our clients and pulling large response without even realising. We want the engineers to deal with the failure first, and only then use this as a tool to temporarily mitigate if required. Often the solution is to fix the bad code pulling such large responses.

- Add `disableWarnings` option to control usage warnings
- Change default `enabled` option to `false`
- Implement usage warnings for unset interceptor configuration
- Update types and documentation to reflect new default behavior
- Enhance flexibility of interceptor configuration
@alexmarqs
Copy link
Member Author

@jpinho I have committed some changes based on your feedback, here is: 0383366

  • Change default enabled option to `false
  • Add disableWarnings global option to control warnings
  • Implement warnings for unset interceptor configuration (no explicit enabled global option set)
  • Update types and documentation to reflect new default behaviour

If all good, I will create a new release and bump the version and then merge. If not, let's keep iterating on this!

Co-authored-by: João Pinho <jpe.pinho@gmail.com>
...DEFAULT_OPTIONS,
...globalOptions,
...configRequestOptions,
} satisfies AxiosLargeResponseOptions;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL!

Copy link
Member

@jpinho jpinho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome contribution @alexmarqs 👏👏👏 ❤️

@alexmarqs alexmarqs merged commit 78b87b7 into epilot-dev:main Mar 6, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants